ci: Add Rust for Linux #2851

ojeda · 2024-06-18T16:05:40Z

Rust for Linux, so far, has pinned the Rust compiler and bindgen versions. The kernel is looking into expanding that support to several versions, i.e. establishing a minimum supported version, so that the kernel can start to be more easily built. In particular, it should be possible to build the kernel using the tools provided directly by Linux distributions. In order to help achieve that goal, the Rust project has added the kernel to its Rust pre-merge CI.

This commit does the same for bindgen. In particular, it adds a quick, build-only test of the Rust code in the kernel as an extra step in the test workflow.

This is intended to be an end-to-end test that runs what kernel developers/users would do. In particular, it is useful to catch certain issues that go beyond the C header comparisons. For instance, it would have been able to catch an issue like the --version option unexpectedly requiring a header in 0.69.0 (fixed in 0.69.1) [1].

It would also have detected another issue present in 0.66.0 and 0.66.1: a panic handling certain C headers with string literals containing an interior NUL [2]. While the kernel is not really a stable test, and such an issue would still require that a proper test is added, it is nevertheless a good test case of non-trivial C headers that may trigger edge cases like that.

Of course, bindgen may need to disable the test for different reasons, i.e. there is no expectation to block any urgent/important PR, and the kernel can also call bindgen differently depending on the version, i.e. we are happy to adjust on our side too. Even if it gets disabled often, we would still be in a better situation than not having the test at all.

The Linux version (hash or tag) should ideally be updated from time to time (e.g. every kernel -rc1), and each update should only contain that change.

Link: #2678 [1]
Link: #2567 [2]

ojeda

Self-review with some comments for reference.

ojeda · 2024-06-18T16:07:08Z

ci/test.sh

+  # Kernel build dependency: `bindgen-cli`, which is under test.
+  #
+  # Using the unstable `--out-dir` so that we can use `cargo build` instead of `cargo install`,
+  # since we want to reuse the `$CARGO_ARGS`.
+  (cd bindgen-cli && RUSTC_BOOTSTRAP=1 cargo build -Zunstable-options --out-dir ${HOME}/.bindgen $CARGO_ARGS)
+  export PATH="${HOME}/.bindgen:${PATH}"


If someone knows a better way to handle this that includes --target and debug/release, please let me know!

Also, I checked in one of my runs that we are actually using the just-built bindgen, but it may be nice to double-check somehow that we are actually using that one and not the other that is already installed in .cargo. I didn't see e.g. the Git hash printed by bindgen in --version --verbose, but that could be a nice way (like in rustc).

ojeda · 2024-06-18T16:09:04Z

.github/workflows/bindgen.yml

@@ -227,6 +227,7 @@ jobs:
          BINDGEN_FEATURE_RUNTIME: ${{matrix.feature_runtime}}
          BINDGEN_FEATURE_EXTRA_ASSERTS: ${{matrix.feature_extra_asserts}}
          BINDGEN_NO_DEFAULT_FEATURES: ${{matrix.no_default_features}}
+          BINDGEN_RUST_FOR_LINUX_TEST: ${{matrix.os == 'ubuntu-latest' && matrix.llvm_version == '16.0' && matrix.feature_extra_asserts == 0 && 1 || 0}}


Perhaps a single additional job should do this. In any case, by doing it in test.sh, it is easy to add it to existing ones or not as needed (and it was nice to test that it actually worked in all the variations of the matrix enabled here).

Also, I disable it under feature_extra_asserts since it seems to take too long / does not finish.

Hi Miguel, do you have any estimates on the time it takes to run this new workflow?

If you mean adding it to the existing jobs, then taking a look at the CI run here and comparing it with the latest, it adds about a minute or two (i.e. to each job in the matrix that enables BINDGEN_RUST_FOR_LINUX_TEST).

If you mean moving it to a new job, then it may run in parallel with the existing jobs (depending on how GitHub handles it). The total time of the new job is similar to existing ones (i.e. 2-3 minutes), as long as one disables BINDGEN_MAIN_TESTS and the other tests above (cargo check, cargo test), since they would be run in the other part of the matrix anyway. So it could be that the total time of the CI does not grow much, depending on GitHub, although we lose doing this end-to-end test with e.g. debug etc.

(Edited the message above to add some more details.)

Which one is the best approach I guess depends on how much you care about the extra minute(s) in existing tests and whether you think running this new part under e.g. debug is worth it etc. I am happy to send the "separate job" one where I added a new BINDGEN_* variable to skip the other bits if needed.

I think we can live with an extra couple minutes here and there, what do you think @emilio? I don't have a strong preference about having this on a completely new job or not. The only question I have is if we want to include this job as a required green check for every PR or if we would be ok with it failing from time to time and fix bindgen before each release.

ojeda · 2024-06-18T16:09:50Z

ci/test.sh

+  # Configure Rust for Linux
+  cat <<EOF > linux/kernel/configs/rfl-for-bindgen-ci.config
+# CONFIG_WERROR is not set


This could be reduced in scope a bit more, but I just did what we are currently doing in the Rust CI, which seemed reasonable (and they should notice any issue there first).

ojeda · 2024-06-18T16:11:00Z

ci/test.sh

+  # Both commit hashes and tags are supported.
+  LINUX_VERSION=c13320499ba0efd93174ef6462ae8a7a2933f6e7


Similarly, I used the same hash here as in the Rust CI, though we could use a tag already if needed.

ojeda · 2024-06-18T16:21:00Z

In e.g. https://github.com/rust-lang/rust-bindgen/actions/runs/9568431855/job/26378640703?pr=2851 you can see at the bottom:

  RUSTC L rust/core.o
  BINDGEN rust/bindings/bindings_generated.rs
  BINDGEN rust/bindings/bindings_helpers_generated.rs
  BINDGEN rust/uapi/uapi_generated.rs
  EXPORTS rust/exports_core_generated.h
  RUSTC P rust/libmacros.so
  RUSTC L rust/compiler_builtins.o
  RUSTC L rust/alloc.o
  RUSTC L rust/bindings.o
  RUSTC L rust/build_error.o
  RUSTC L rust/uapi.o
  EXPORTS rust/exports_alloc_generated.h
  EXPORTS rust/exports_bindings_generated.h
  RUSTC L rust/kernel.o
  EXPORTS rust/exports_kernel_generated.h
  RUSTDOC TK rust/kernel/lib.rs
  RUSTC [M] samples/rust/rust_minimal.o
  RUSTC     samples/rust/rust_print.o
  RUSTC     drivers/net/phy/ax88796b_rust.o

emilio · 2024-06-18T16:19:02Z

ci/test.sh

+  #
+  # Using the unstable `--out-dir` so that we can use `cargo build` instead of `cargo install`,
+  # since we want to reuse the `$CARGO_ARGS`.
+  (cd bindgen-cli && RUSTC_BOOTSTRAP=1 cargo build -Zunstable-options --out-dir ${HOME}/.bindgen $CARGO_ARGS)


It seems you wouldn't need RUSTC_BOOTSTRAP etc if you use CARGO_TARGET_DIR=$HOME/.bindgen or so?

https://doc.rust-lang.org/cargo/reference/environment-variables.html#environment-variables-cargo-reads

Apparently the --target-dir flag is also not unstable.

Ah, ok so I see what you're doing here, you just want to make sure you get the target-dir/debug/ or release right...

Maybe given we're only interested on running this on linux (so no explicit --target), and we know whether we're doing a release build (via BINDGEN_RELEASE_BUILD), maybe just cargo build $CARGO_ARGS, and then either conditionally add target/debug or target/release to the PATH, or maybe just add both to the PATH?

Yeah, I did this to be able to use whatever you had in $CARGO_ARGS (and cargo install does not accept --release), and it seemed like the clean way, but sadly unstable.

The --target should not matter for this CI as you say, but I guess you could be adding different profiles in the future. If that is not a concern, then simply adding both debug/release to $PATH sounds the simplest.

(It could still "break" or be surprising if other things get refactored and both debug and release get built in the same test for something else above. We could use --target-dir to at least isolate that).

Rust for Linux, so far, has pinned the Rust compiler and `bindgen` versions. The kernel is looking into expanding that support to several versions, i.e. establishing a minimum supported version, so that the kernel can start to be more easily built. In particular, it should be possible to build the kernel using the tools provided directly by Linux distributions. In order to help achieve that goal, the Rust project has added the kernel to its Rust pre-merge CI. This commit does the same for `bindgen`. In particular, it adds a quick, build-only test of the Rust code in the kernel as an extra step in the `test` workflow. This is intended to be an end-to-end test that runs what kernel developers/users would do. In particular, it is useful to catch certain issues that go beyond the C header comparisons. For instance, it would have been able to catch an issue like the `--version` option unexpectedly requiring a header in 0.69.0 (fixed in 0.69.1) [1]. It would also have detected another issue present in 0.66.0 and 0.66.1: a panic handling certain C headers with string literals containing an interior NUL [2]. While the kernel is not really a stable test, and such an issue would still require that a proper test is added, it is nevertheless a good test case of non-trivial C headers that may trigger edge cases like that. Of course, `bindgen` may need to disable the test for different reasons, i.e. there is no expectation to block any urgent/important PR, and the kernel can also call `bindgen` differently depending on the version, i.e. we are happy to adjust on our side too. Even if it gets disabled often, we would still be in a better situation than not having the test at all. The Linux version (hash or tag) should ideally be updated from time to time (e.g. every kernel `-rc1`), and each update should only contain that change. Link: rust-lang#2678 [1] Link: rust-lang#2567 [2] Signed-off-by: Miguel Ojeda <[email protected]>

ojeda · 2024-06-19T11:14:42Z

v2: instead of the unstable --out-dir, put both the common profiles in $PATH and use --target-dir to isolate a bit.

(Using --target-dir implies we rebuild more than needed, since some crates may be already built by the previous tests)

With both the workaround for `bindgen` 0.69.0 and the warning about 0.66.0 and 0.66.1 in place, start supporting several `bindgen` versions, like it was done for the Rust compiler in a previous patch. All other versions, including the latest 0.69.4, build without errors. The `bindgen` project, like Rust, has also agreed to have the kernel in their CI [1] -- thanks! This should help both projects: `bindgen` will be able to detect early issues like those mentioned above, and the kernel will be very likely build with new releases (at least for the basic configuration being tested). Link: rust-lang/rust-bindgen#2851 [1] Signed-off-by: Miguel Ojeda <[email protected]>

With both the workaround for `bindgen` 0.69.0 and the warning about 0.66.0 and 0.66.1 in place, start supporting several `bindgen` versions, like it was done for the Rust compiler in a previous patch. All other versions, including the latest 0.69.4, build without errors. The `bindgen` project, like Rust, has also agreed to have the kernel in their CI [1] -- thanks! This should help both projects: `bindgen` will be able to detect early issues like those mentioned above, and the kernel will be very likely build with new releases (at least for the basic configuration being tested). Link: rust-lang/rust-bindgen#2851 [1] Signed-off-by: Miguel Ojeda <[email protected]> Link: https://lore.kernel.org/r/[email protected]

With both the workaround for `bindgen` 0.69.0 and the warning about 0.66.0 and 0.66.1 in place, start supporting several `bindgen` versions, like it was done for the Rust compiler in a previous patch. All other versions, including the latest 0.69.4, build without errors. The `bindgen` project, like Rust, has also agreed to have the kernel in their CI [1] -- thanks! This should help both projects: `bindgen` will be able to detect early issues like those mentioned above, and the kernel will be very likely build with new releases (at least for the basic configuration being tested). Link: rust-lang/rust-bindgen#2851 [1] Signed-off-by: Miguel Ojeda <[email protected]> Tested-by: Benno Lossin <[email protected]>

With both the workaround for `bindgen` 0.69.0 and the warning about 0.66.0 and 0.66.1 in place, start supporting several `bindgen` versions, like it was done for the Rust compiler in a previous patch. All other versions, including the latest 0.69.4, build without errors. The `bindgen` project, like Rust, has also agreed to have the kernel in their CI [1] -- thanks! This should help both projects: `bindgen` will be able to detect early issues like those mentioned above, and the kernel will be very likely build with new releases (at least for the basic configuration being tested). Link: rust-lang/rust-bindgen#2851 [1] Signed-off-by: Miguel Ojeda <[email protected]> Link: https://lore.kernel.org/r/[email protected]

With both the workaround for `bindgen` 0.69.0 and the warning about 0.66.0 and 0.66.1 in place, start supporting several `bindgen` versions, like it was done for the Rust compiler in a previous patch. All other versions, including the latest 0.69.4, build without errors. The `bindgen` project, like Rust, has also agreed to have the kernel in their CI [1] -- thanks! This should help both projects: `bindgen` will be able to detect early issues like those mentioned above, and the kernel will be very likely build with new releases (at least for the basic configuration being tested). Link: rust-lang/rust-bindgen#2851 [1] Tested-by: Benno Lossin <[email protected]> Tested-by: Andreas Hindborg <[email protected]> Signed-off-by: Miguel Ojeda <[email protected]>

With both the workaround for `bindgen` 0.69.0 and the warning about 0.66.0 and 0.66.1 in place, start supporting several `bindgen` versions, like it was done for the Rust compiler in a previous patch. All other versions, including the latest 0.69.4, build without errors. The `bindgen` project, like Rust, has also agreed to have the kernel in their CI [1] -- thanks! This should help both projects: `bindgen` will be able to detect early issues like those mentioned above, and the kernel will be very likely build with new releases (at least for the basic configuration being tested). Link: rust-lang/rust-bindgen#2851 [1] Tested-by: Benno Lossin <[email protected]> Tested-by: Andreas Hindborg <[email protected]> Signed-off-by: Miguel Ojeda <[email protected]> Link: https://lore.kernel.org/r/[email protected]

With both the workaround for `bindgen` 0.69.0 and the warning about 0.66.0 and 0.66.1 in place, start supporting several `bindgen` versions, like it was done for the Rust compiler in a previous patch. All other versions, including the latest 0.69.4, build without errors. The `bindgen` project, like Rust, has also agreed to have the kernel in their CI [1] -- thanks! This should help both projects: `bindgen` will be able to detect early issues like those mentioned above, and the kernel will be very likely build with new releases (at least for the basic configuration being tested). Link: rust-lang/rust-bindgen#2851 [1] Tested-by: Benno Lossin <[email protected]> Tested-by: Andreas Hindborg <[email protected]> Signed-off-by: Miguel Ojeda <[email protected]>

With both the workaround for `bindgen` 0.69.0 and the warning about 0.66.0 and 0.66.1 in place, start supporting several `bindgen` versions, like it was done for the Rust compiler in a previous patch. All other versions, including the latest 0.69.4, build without errors. The `bindgen` project, like Rust, has also agreed to have the kernel in their CI [1] -- thanks! This should help both projects: `bindgen` will be able to detect early issues like those mentioned above, and the kernel will be very likely build with new releases (at least for the basic configuration being tested). Link: rust-lang/rust-bindgen#2851 [1] Tested-by: Benno Lossin <[email protected]> Tested-by: Andreas Hindborg <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Miguel Ojeda <[email protected]>

pvdrz · 2024-08-30T20:54:48Z

@ojeda would be reasonable to run this job on a cron, let's say, once every week?

ojeda · 2024-08-31T11:02:27Z

Up to you of course -- did something change?

If the extra couple minutes would be too much even in a new job, then a periodic post-merge CI is better than nothing. The disadvantage would be that one would find the issues after the fact, which is more painful overall (e.g. may require figuring out which commit actually broke it and whether a revert is needed and whether that breaks something else, the PR author may not be around to help, etc.).

With both the workaround for `bindgen` 0.69.0 and the warning about 0.66.0 and 0.66.1 in place, start supporting several `bindgen` versions, like it was done for the Rust compiler in a previous patch. All other versions, including the latest 0.69.4, build without errors. The `bindgen` project, like Rust, has also agreed to have the kernel in their CI [1] -- thanks! This should help both projects: `bindgen` will be able to detect early issues like those mentioned above, and the kernel will be very likely build with new releases (at least for the basic configuration being tested). Link: rust-lang/rust-bindgen#2851 [1] Tested-by: Benno Lossin <[email protected]> Tested-by: Andreas Hindborg <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Miguel Ojeda <[email protected]>

ojeda mentioned this pull request Jun 18, 2024

bindgen wanted features & bugfixes Rust-for-Linux/linux#353

Open

23 tasks

ojeda commented Jun 18, 2024

View reviewed changes

ojeda added the rust-for-linux Issues relevant to the Rust for Linux project label Jun 18, 2024

emilio reviewed Jun 18, 2024

View reviewed changes

ojeda force-pushed the rfl-ci branch from b2d1512 to 61ddde7 Compare June 19, 2024 11:10

pvdrz mentioned this pull request Jul 18, 2024

Idea: Add firefox to CI #2870

Open

pvdrz approved these changes Sep 19, 2024

View reviewed changes

pvdrz added this pull request to the merge queue Sep 19, 2024

Merged via the queue into rust-lang:main with commit d39fc1b Sep 19, 2024
33 checks passed

ojeda deleted the rfl-ci branch September 23, 2024 18:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: Add Rust for Linux #2851

ci: Add Rust for Linux #2851

ojeda commented Jun 18, 2024

ojeda left a comment

ojeda Jun 18, 2024

ojeda Jun 18, 2024

ojeda Jun 18, 2024 •

edited

Loading

pvdrz Jun 18, 2024

ojeda Jun 19, 2024 •

edited

Loading

ojeda Jun 19, 2024

pvdrz Jun 20, 2024

ojeda Jun 18, 2024

ojeda Jun 18, 2024

ojeda commented Jun 18, 2024

emilio Jun 18, 2024

emilio Jun 18, 2024

emilio Jun 18, 2024

ojeda Jun 18, 2024 •

edited

Loading

ojeda commented Jun 19, 2024 •

edited

Loading

pvdrz commented Aug 30, 2024

ojeda commented Aug 31, 2024

		# Both commit hashes and tags are supported.
		LINUX_VERSION=c13320499ba0efd93174ef6462ae8a7a2933f6e7

ci: Add Rust for Linux #2851

ci: Add Rust for Linux #2851

Conversation

ojeda commented Jun 18, 2024

ojeda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ojeda Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ojeda Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ojeda commented Jun 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ojeda Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

ojeda commented Jun 19, 2024 • edited Loading

pvdrz commented Aug 30, 2024

ojeda commented Aug 31, 2024

ojeda Jun 18, 2024 •

edited

Loading

ojeda Jun 19, 2024 •

edited

Loading

ojeda Jun 18, 2024 •

edited

Loading

ojeda commented Jun 19, 2024 •

edited

Loading